Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update model for interfaces extending interfaces #4663

Merged
merged 6 commits into from
Nov 13, 2024
Merged

Update model for interfaces extending interfaces #4663

merged 6 commits into from
Nov 13, 2024

Conversation

rlsanders4
Copy link
Contributor

@rlsanders4 rlsanders4 commented Nov 11, 2024 •
edited
Loading

What's changed?

Update the parser model to correctly represent an interface extending another interface.

What's your motivation?

Currently, when a Java interface extends another interface it is represented in the resulting J.ClassDeclaration as an 'implements' rather than an 'extends'. This can cause confusion when authoring custom recipes, because instead of accessing the extended interface using J.ClassDeclaration.getExtends() it must instead be accesed using J.ClassDeclaration.getImplements().

For example:

Parent interface

package parent;
public interface parentInterface {}

Child interface

package child;
public interface childInterface extends parentInterface {}

Once the child interface has been parsed, in a visitClassDeclaration() visitor we would expect to be able to access the extended parent interface with cd.getExtends(). However, that currently returns null, and you must access it instead with cd.getImplements().

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek
Copy link
Contributor

Saving others a click: we fail on line 50 with

        // Check that childInterface does not implement any interfaces
        assertNull(cd.getImplements());

ExtendInterfaceTest > interfaceExtendsInterface() FAILED
    org.opentest4j.AssertionFailedError: expected: <null> but was: <[parentInterface]>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertNull.failNotNull(AssertNull.java:50)
        at app//org.junit.jupiter.api.AssertNull.assertNull(AssertNull.java:35)
        at app//org.junit.jupiter.api.AssertNull.assertNull(AssertNull.java:30)
        at app//org.junit.jupiter.api.Assertions.assertNull(Assertions.java:279)
        at app//org.openrewrite.java.ExtendInterfaceTest.interfaceExtendsInterface(ExtendInterfaceTest.java:50)

@rlsanders4 rlsanders4 changed the title Add interface extending interface test Update model for interfaces extending interfaces Nov 11, 2024
@rlsanders4 rlsanders4 marked this pull request as ready for review November 11, 2024 22:25
@rlsanders4 rlsanders4 self-assigned this Nov 11, 2024
@timtebeek timtebeek added bug Something isn't working parser-java labels Nov 11, 2024
@timtebeek
Copy link
Contributor

Thanks for diving in here @rlsanders4 ! Surprised no one else had stumbled upon this before; Now I'm wondering if anyone had worked around this, although recipes for interfaces that extend interfaces might be rare.

As a note to myself we should likely add the same changes in the other Java parsers, just so this is consistent when parsed and targeted there on Java 8, 11 and 21 as well.

@timtebeek timtebeek marked this pull request as draft November 12, 2024 22:58
timtebeek
timtebeek requested changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capturing some of what was discussed internally here as well: Yes it's confusing that extends/implements are intuitively the wrong way round, but it matches how these are modeled in the Java compiler as well. In part this might be due to interfaces being able to extend multiple other interfaces, whereas classes can only extend one class, but can implement many interfaces. Changing any part of that now it likely to be painful given that we serialize LSTs. Perhaps the best way forward is to document this seeming inversion for interfaces on the getExtends and getImplements methods for class declarations.

@rlsanders4 rlsanders4 marked this pull request as ready for review November 13, 2024 17:45
timtebeek
timtebeek approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working through this with us to improve the experience for folks going forward!

@timtebeek timtebeek merged commit 9997b5c into openrewrite:main Nov 13, 2024
2 checks passed
@rlsanders4 rlsanders4 deleted the extendInterface branch November 13, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timtebeek timtebeek timtebeek approved these changes

Assignees

@rlsanders4 rlsanders4

Labels
bug Something isn't working parser-java
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rlsanders4 @timtebeek